-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Gracefully handle concurrent zone decommission action #5542
Gracefully handle concurrent zone decommission action #5542
Conversation
Signed-off-by: Rishab Nahata <rnnahata@amazon.com>
Signed-off-by: Rishab Nahata <rnnahata@amazon.com>
Signed-off-by: Rishab Nahata <rnnahata@amazon.com>
Gradle Check (Jenkins) Run Completed with:
|
Codecov Report
@@ Coverage Diff @@
## main #5542 +/- ##
============================================
+ Coverage 70.98% 71.05% +0.06%
- Complexity 58669 58685 +16
============================================
Files 4763 4764 +1
Lines 279945 280022 +77
Branches 40418 40425 +7
============================================
+ Hits 198731 198958 +227
+ Misses 64988 64750 -238
- Partials 16226 16314 +88
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Rishab Nahata <rnnahata@amazon.com>
Gradle Check (Jenkins) Run Completed with:
|
Failing due to version bump after 2.4.1 release, #5560. Fixed is merged into main branch. @imRishN : Can you please rebase your branch against latest
|
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Rishab Nahata <rnnahata@amazon.com>
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
...java/org/opensearch/action/admin/cluster/decommission/awareness/put/DecommissionRequest.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/cluster/decommission/DecommissionService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/cluster/decommission/DecommissionService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/cluster/decommission/DecommissionAttributeMetadata.java
Show resolved
Hide resolved
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Rishab Nahata <rnnahata@amazon.com>
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
server/src/main/java/org/opensearch/cluster/decommission/DecommissionAttributeMetadata.java
Show resolved
Hide resolved
@@ -144,12 +145,19 @@ public void startDecommissionAction( | |||
public ClusterState execute(ClusterState currentState) { | |||
// validates if correct awareness attributes and forced awareness attribute set to the cluster before starting action | |||
validateAwarenessAttribute(decommissionAttribute, awarenessAttributes, forcedAwarenessAttributes); | |||
if (decommissionRequest.requestID() == null) { | |||
decommissionRequest.setRequestID(UUIDs.randomBase64UUID()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of setting here. Just generate it in RestAction so that you don't need it set it again. Then your request would also be immutable. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, only the REST layer would be having UUIDs set. We would want the IDs to be there not conditionally. When client pkgs tries to interact with request object directly, won't it be a problem? Also Integ tests are not Rest layer tests. And OpenSearch uses http smoke test pkg for rest integ tests. The current IT pkg doesn't support REST tests. Let me know if this makes sense..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@imRishN I was wondering if you set the requestId here itself -
OpenSearch/server/src/main/java/org/opensearch/rest/action/admin/cluster/RestDecommissionAction.java
Line 49 in b71caae
DecommissionRequest createRequest(RestRequest request) throws IOException { |
if (decommissionRequest.requestID().equals(decommissionAttributeMetadata.requestID()) == false) { | ||
throw new DecommissioningFailedException( | ||
requestedDecommissionAttribute, | ||
"same request is already in status [INIT]" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[minor]: another request instead of same request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a same request from users perspective as it would be decommissioning same attribute name and attribute value. Although, technically its a different request but these messages are generally user friendly
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @imRishN for the changes.
Gradle Check (Jenkins) Run Completed with:
|
…ject#5542) * Control concurrency and handle retries during decommissioning of awareness attributes Signed-off-by: Rishab Nahata <rnnahata@amazon.com>
…ject#5542) * Control concurrency and handle retries during decommissioning of awareness attributes Signed-off-by: Rishab Nahata <rnnahata@amazon.com>
Description
This PR introduces changes related to Decommission Service -
Issues Resolved
#4543
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.